fix: isolate empty-mention waiter by sender without changing default session scope - #9378
fix: isolate empty-mention waiter by sender without changing default session scope#9378hedssaz wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
xiaoyuyu6420
left a comment
There was a problem hiding this comment.
Review
I've reviewed this PR in the context of issue #9377 alongside the two other open PRs (#9422, #9442) that address the same bug. Here's my analysis.
Strengths
-
Length-prefixed key encoding (
{len(umo)}:{umo}:{sender_id}) — this is technically the most robust approach to key collision resistance. Telegram topic-group origins contain#, and:appears in all UMO formats (platform:MessageType:id), so plain concatenation with a separator has a theoretical collision surface. This encoding is uniquely decodable. -
Comprehensive test suite — two separate test files covering both unit-level key logic and integration-level
Main.handle_empty_mentionwiring. The regression guard forastrbot_plugin_kimi_datasource_api(manual UMO registration pattern) is particularly valuable. -
Opt-in design preserves backward compatibility —
DefaultSessionFilteris unchanged, so third-party plugins that rely on group-level keys are not affected.
Issues
1. Does not fix the non-text message swallowing bug (mentioned in #9377 as a "连带问题")
The issue report explicitly documents that pure image messages during the wait window are silently dropped and the waiter stays alive:
empty_mention_waiter handler:
if not event.message_str or not event.message_str.strip():
return # ← pure image is silently dropped, waiter keeps runningThis PR only adds SenderSessionFilter to the call site but does not modify the handler body. After this fix, Alice's pure image in the wait window is still silently consumed (by Alice herself now, not by Bob — an improvement, but the message is still lost).
2. Does not fix the _cleanup race condition (mentioned in #9377 as a "连带问题")
The issue report documents that SessionWaiter._cleanup() unconditionally calls USER_SESSIONS.pop(self.session_id). If a newer waiter is registered under the same key before the old one times out, the old waiter's cleanup evicts the new one. This PR does not address this.
3. Skipping the waiter on empty sender_id is a behavioral regression
if str(event.get_sender_id()):
await empty_mention_waiter(event, session_filter=SenderSessionFilter())When sender_id is empty/falsy, the waiter is simply not started. On platforms where get_sender_id() may temporarily return empty, the bot silently stops responding to empty mentions with no feedback to the user. A fallback placeholder (e.g., <unknown>) with a warning log would be safer — it degrades gracefully instead of silently dropping functionality.
4. Only fixes empty_mention_waiter call site, not is_wake_prefix_only
The issue report notes: "仅含唤醒前缀的消息(is_wake_prefix_only 分支)会启动同一个等待器,跨用户截获问题同样适用于该场景". The same session_waiter is used in both branches, but this PR only passes SenderSessionFilter to the empty_mention_waiter call. The wake-prefix-only path still uses DefaultSessionFilter and is still vulnerable to cross-user interception.
Summary
This PR correctly identifies the core isolation problem and proposes a clean opt-in API with excellent test coverage. However, it addresses only one of the three issues documented in #9377 (cross-user interception), leaving the non-text swallowing and cleanup race unfixed. The empty-sender-id skip also introduces a minor behavioral regression.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8597ae2751
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| async def finish(task: asyncio.Task) -> None: | ||
| task.cancel() |
There was a problem hiding this comment.
Inline the single-use task cleanup
finish() is called only once and merely wraps a short, linear cancellation block, so extracting it fragments the test without meeting the repository’s requirement that helpers have at least three reuse sites or prevent extreme complexity. Inline this cleanup at its call site instead.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38e036c065
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return main | ||
|
|
||
|
|
||
| async def start_empty_mention(main: Main, event: FakeEvent) -> asyncio.Task: |
There was a problem hiding this comment.
Inline the two-use waiter startup helper
start_empty_mention() is used by only two tests and wraps a short startup sequence, while also introducing the single-use nested drain() helper. Neither helper reaches the required three reuse sites or avoids extreme complexity, so inline this setup into the two tests instead.
AGENTS.md reference: AGENTS.md:L65-L73
Useful? React with 👍 / 👎.
Fixes #9377
In group chats, an empty mention (
@botwith no text) starts a 60ssession_waiterto wait for follow-up content. The waiter is registered withDefaultSessionFilter, whose key isunified_msg_origin(group-level) and doesnot include the sender. As a result, the first non-empty message from any
other member during the wait window is intercepted, gets a bot
Atprepended,and is re-dispatched — producing a reply to the wrong user / wrong context. Full
analysis, reproduction and captured logs are in #9377.
This PR fixes the empty-mention flow without changing the default session
scope, so third-party plugins that rely on the current group-level default are
unaffected.
Why not simply change
DefaultSessionFilterChanging the default from
unified_msg_originto include the sender wouldchange the default runtime behavior of
session_waiter, which is exported toplugins through
astrbot.api.util. The group-level default has been in placesince v3.5.4 (~15 months, 138 stable releases) and is relied upon by real
plugins that omit
session_filter=, for example:astrbot_plugin_kimi_datasource_apiregisters a waiter manually withSessionWaiter(DefaultSessionFilter(), event.unified_msg_origin, ...). Theregistration key is the raw UMO while lookups go through
DefaultSessionFilter.filter(event). If the default changed, the keys would nolonger match and its login waiter would never trigger.
astrbot_plugin_decision_rouletteandastrbot_plugin_TurtleSoupuse thedefault filter to collect input from multiple group members; a per-sender
default would only accept the initiator.
So the fix is scoped to the one place that actually needs per-sender isolation.
Modifications
astrbot/core/utils/session_waiter.py: addSenderSessionFilter, a newoptional
SessionFilterkeyed byunified_msg_origin+sender_id.DefaultSessionFilteris unchanged, and no new names are added to thepublic
astrbot.api.utilsurface. This is purely additive.The key uses a length-prefixed encoding (
f"{len(umo)}:{umo}:{sender}")rather than plain concatenation: both the UMO and the sender id are arbitrary
platform strings (e.g. Telegram topic-group UMOs contain
#), so a plainseparator would allow constructible key collisions that defeat the isolation.
astrbot/builtin_stars/astrbot/main.py: the empty-mention waiter now passessession_filter=SenderSessionFilter(). When the sender id is unavailable itskips waiting entirely, to avoid silently degrading back to group scope.
tests/test_session_waiter_cross_user.py(new): 7 session-key regressiontests (module loaded standalone).
tests/unit/test_empty_mention_sender_scope.py(new): 3 production-wiringtests driving the real
Main.handle_empty_mentionandMain.handle_session_control_agent(following the existingMain.__new__(Main)pattern intests/unit/test_group_chat_context_wiring.py).The other issues noted in #9377 (empty-
message_strmessages being swallowedduring the wait window;
_cleanup/FILTERSbehavior with same-key waiters)are independent of the session key and will be handled in separate PRs.
Screenshots or Test Results
Session-key coverage (
tests/test_session_waiter_cross_user.py):test_default_filter_returns_umo_unchanged— the default filter still returnsunified_msg_origin(locks the default semantics in place);test_default_manual_umo_registration_still_triggers— reproduces thekimi_datasource_apimanual-registration pattern and asserts it stilltriggers (guards against regressing plugins that depend on the group default);
test_sender_filter_isolates_users_in_group— another member's message is nolonger intercepted (primary goal);
test_sender_filter_accepts_original_sender— the original sender's follow-upis still captured;
test_sender_filter_key_is_collision_free— UMOs / sender ids containingseparator characters (
#,:) cannot produce colliding keys;test_sender_filter_isolates_users_with_hash_in_umo— isolation holds forTelegram topic-group style UMOs that contain
#;test_sender_filter_does_not_leak_across_conversations— the same user'smessages in other groups / DMs are not intercepted.
Production-wiring coverage (
tests/unit/test_empty_mention_sender_scope.py,drives the real handlers rather than a re-implementation):
test_other_members_message_passes_through— BOB's message is not stopped,not modified, and not re-enqueued while ALICE's waiter is active;
test_original_senders_followup_is_redispatched— ALICE's follow-up gets thebot
Atinjected, is stopped and re-enqueued, and the waiter is cleaned up;test_empty_sender_id_skips_waiting— with no reliable sender id thehandler finishes immediately and registers no waiter.
Verified that the wiring tests actually guard the fix: with the
main.pychange reverted to master, tests 8 and 10 fail; with the fix in place all 10
pass.
Checklist
via issues/emails. (Bug fix, proposed in [Bug] 群聊中空提及等待器会截获其他成员的消息(session_waiter 未绑定发送者) #9377.)
provided above.
Summary by Sourcery
Fix empty-mention session handling in group chats by isolating the waiter per sender while preserving existing group-level session semantics.
Bug Fixes:
Enhancements:
Tests: